-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] implented confirm password #31
[feat] implented confirm password #31
Conversation
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should be removed from the PR since we're only using pnpm
? you could try running
git checkout main -- [path-to-package-lock.json]
then commit and push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! or u can directly just delete the entire file
app/(auth)/signup/page.tsx
Outdated
password={password || ''} // Set default value if password is null | ||
/> | ||
{/* Password complexity requirements */} | ||
{password && passwordComplexity === false && passwordComplexityError && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could simplify this to {password && !passwordComplexity && passwordComplexityError ...
const [passwordComplexityError, setPasswordComplexityError] = useState< | ||
string | null | ||
>(null); | ||
const [passwordComplexity, setPasswordComplexity] = useState<boolean>(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be easier to change these variables to more descriptive names? lowkey got a bit confused haha, maybe isPasswordComplexityMet
or something similar would help to differentiate it from passwordComplexityError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! agreed about renaming
looks great so far! waiting on comments from Catherine... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's looking so great rachael 🤩 ! added some more small nits, but mostly ready to go!
- add icons from figma
- correct placement of PasswordComplexity component
- remove null as a possible value for the password and confirmPassword states
- both PasswordInput.tsx and PasswordComplexity.tsx should be in /components rather than in /app/utils
app/dashboard/page.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment on the AuthContext PR - let's comment this out and remove this file since we don't want this route to be active on the app rn.
app/utils.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's delete this file if it's empty!
app/utils/PasswordComplexity.tsx
Outdated
function Requirement({ met, text }: { met: boolean; text: string }) { | ||
return ( | ||
<p style={{ color: met ? 'green' : 'red' }}> | ||
{met ? '✓' : '✗'} {text} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! this is all right for now, but we eventually want to use the icons from figma.
i'd recommend exporting greenCheck and redCross icons as svg's from figma and adding them to the project. If you rebase from main, you'll be able to use the icon component i added!
package.json
Outdated
@@ -14,9 +14,10 @@ | |||
"pre-commit": "(pnpm run tsc || true) && (pnpm run lint || true) && pnpm run prettier" | |||
}, | |||
"dependencies": { | |||
"next": "^14.2.10", | |||
"@next/swc-darwin-arm64": "^15.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line should be deleted!
app/utils/PasswordInput.tsx
Outdated
name, | ||
}) => { | ||
return ( | ||
<div style={{ position: 'relative' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually we want to replace the inline styling with Styled Components
)} | ||
{/* Conditional password validation error message */} | ||
<PasswordComplexity | ||
password={password || ''} // Set default value if password is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if password is not able to be of type null, we don't need this additional password || ''
and we can simplify to password={password}
app/utils/PasswordInput.tsx
Outdated
import React from 'react'; | ||
|
||
interface PasswordInputProps { | ||
value: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the null typing!
app/utils/PasswordInput.tsx
Outdated
|
||
interface PasswordInputProps { | ||
value: string | null; | ||
onChange: (e: React.ChangeEvent<HTMLInputElement>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be simplified to
onChange: (password: string) => void
if that helps (as long as u change line 25 accordingly to
onChange={(e)=>onChange(e.target.value)}
<p style={{ color: 'red' }}>{passwordError}</p> | ||
)} | ||
{/* Conditional password validation error message */} | ||
<PasswordComplexity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while the "Passwords Do Not Match" error is in the correct place rn, to match designs, PasswordComplexity
should be directly under the Password input (i.e. above Confirm Password)
app/utils/PasswordInput.tsx
Outdated
name: string; | ||
} | ||
|
||
const PasswordInput: React.FC<PasswordInputProps> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's change this to the standard format of component declaration:
export default function PassowordInput(
{value,
onChange,
placeholder,
isVisible,
toggleVisibility,
name}:
PasswordInputProps) {
...
}
setPasswordComplexityError(null); // Clear error if all conditions are met | ||
} else if (password) { | ||
setPasswordComplexity(false); | ||
setPasswordComplexityError('Password must meet complexity requirements'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging that it makes more sense for this error to only show up if user tries to submit without meeting requirements. it's fine for now but we may want to adjust the logic in the future
app/(auth)/login/page.tsx
Outdated
|
||
push('/'); | ||
|
||
return data; | ||
}; | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to make the outermost container a form
to improve the accessibility of this page.
here's some skeleton code from chatGPT
import React, { useState } from "react";
export default function Signup() {
const [email, setEmail] = useState("");
const [password, setPassword] = useState("");
const [confirmPassword, setConfirmPassword] = useState("");
const [errorMessage, setErrorMessage] = useState("");
const isFormValid = email && password && confirmPassword && password === confirmPassword;
const handleSignUp = (e) => {
e.preventDefault(); // Prevent default form submission behavior
// Clear error message and proceed with sign-up logic
setErrorMessage("");
// Add your sign-up logic here
};
return (
<form onSubmit={handleSignUp}>
<input
name="email"
onChange={(e) => setEmail(e.target.value)}
value={email}
placeholder="Email"
required
/>
<input
type="password"
name="password"
onChange={(e) => setPassword(e.target.value)}
value={password}
placeholder="Password"
required
/>
<input
type="password"
name="confirmPassword"
onChange={(e) => setConfirmPassword(e.target.value)}
value={confirmPassword}
placeholder="Confirm Password"
required
/>
{errorMessage && <p style={{ color: "red" }}>{errorMessage}</p>}
<button type="submit" disabled={!isFormValid}>
Sign up
</button>
</form>
);
}
ut, complexity check, and toggle password visibility features
a542f70
to
c2f53ae
Compare
What's new in this PR 🧑🌾
Description
Screenshots
No checks passed
Some checks passed
Complexity checks passed but passwords don't match
All checks passed and signup button is enabled
Toggle visibility
How to review
Next steps
Relevant links
Online sources
Related PRs
CC: @ccatherinetan